Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

fix(DASH): Fix playback after DASH period eviction #7519

Merged
merged 8 commits into from
Nov 13, 2024
Merged

fix(DASH): Fix playback after DASH period eviction #7519

merged 8 commits into from
Nov 13, 2024

Conversation

ncocaign
Copy link
Contributor

@ncocaign ncocaign commented Oct 29, 2024 •
edited by joeyparrish
Loading

With this change, closeSegmentIndex() of all streams of a removed period are defered in StreamingEngine.onUpdate_()

Fixes #7516

BEGIN_COMMIT_OVERRIDE
ignore: Reverted
END_COMMIT_OVERRIDE

avelad
avelad requested changes Oct 29, 2024
View reviewed changes
some-changes.patch Outdated Show resolved Hide resolved
@ncocaign ncocaign requested a review from avelad October 30, 2024 17:00
@shaka-bot
Copy link
Collaborator

Incremental code coverage: 98.08%

@ncocaign ncocaign changed the title fix(DASH): Fix playback of some DASH with multiple period (#7516) fix(DASH): Fix playback of some DASH with multiple periods (#7516) Oct 31, 2024
@ncocaign ncocaign requested a review from joeyparrish October 31, 2024 09:06
@avelad avelad added type: bug Something isn't working correctly priority: P2 Smaller impact or easy workaround component: DASH The issue involves the MPEG DASH manifest format labels Oct 31, 2024
@avelad avelad added this to the v4.12 milestone Oct 31, 2024
@avelad avelad requested review from theodab and tykus160 October 31, 2024 09:06
@avelad avelad changed the title fix(DASH): Fix playback of some DASH with multiple periods (#7516) fix(DASH): Fix playback of some DASH with multiple periods Nov 4, 2024
tykus160
tykus160 requested changes Nov 4, 2024
View reviewed changes
Copy link
Member

@tykus160 tykus160 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you add tests?

build/types/core Outdated Show resolved Hide resolved
externs/shaka/manifest_parser.js Outdated Show resolved Hide resolved
lib/dash/dash_parser.js Outdated Show resolved Hide resolved
lib/media/segment_index.js Outdated Show resolved Hide resolved
@avelad avelad added the status: waiting on response Waiting on a response from the reporter(s) of the issue label Nov 6, 2024
@ncocaign
Copy link
Contributor Author

thanks for the feedback, I'm making the various modifications.
I would like to tell your opinions on whether having defered the closing of the segmentIndexes at the level of the StreamingEngine suits you.
Especially in order to not break the management of the indexes in shaka.media.SegmentIndex the evicted segment counter has been updated with evictAll().

@shaka-bot shaka-bot removed the status: waiting on response Waiting on a response from the reporter(s) of the issue label Nov 12, 2024
@avelad avelad modified the milestones: v4.12, v4.13 Nov 13, 2024
avelad
avelad requested changes Nov 13, 2024
View reviewed changes
Copy link
Member

@avelad avelad left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please rebase your PR against the main branch and fix the conflicts. Thanks!

externs/shaka/manifest_parser.js Outdated Show resolved Hide resolved
lib/media/segment_index.js Outdated Show resolved Hide resolved
lib/media/streaming_engine.js Outdated Show resolved Hide resolved
lib/media/streaming_engine.js Outdated Show resolved Hide resolved
lib/media/streaming_engine.js Outdated Show resolved Hide resolved
lib/media/streaming_engine.js Outdated Show resolved Hide resolved
lib/util/periods.js Outdated Show resolved Hide resolved
@ncocaign
Copy link
Contributor Author

Could you add tests?

Difficult for me to add test as the issues arrive in a very specific condition (switch + segmentUpdate + period eviction in manifest update).

@ncocaign
fix(DASH): Fix playback of some DASH with multiple period (shaka-proj…
727966c 
…ect#7516)

With this change, closeSegmentIndex() of all streams of a removed period are defered in StreamingEngine.onUpdate_()

Fixes shaka-project#7516
@ncocaign
add shaka.util.CloseSegmentIndexRegister
7e7724f 
Provide an interface between StreamEngine and ManifestParser to defer the call of closeStreamIndex durring the update of segments instead the update of the manifest.

Fix eslint / jsdoc
@ncocaign
fix eslint
2828459
@ncocaign
remove patch from previous version
117525e
@ncocaign
Style nit: indent +2
12879f5
@ncocaign
Add player interface to close segmentIndex from manifest
834d71e 
extend shaka.extern.ManifestParser.PlayerInterface to provide the function closeSegmentIndex.
SegmentIndexex are closed only if not part of an active stream.
@ncocaign
PR review
697c17e 
restore empty lines initialy removed by eslint --fix
remove logExtPrefix_
@ncocaign ncocaign force-pushed the fix-dash-multiperiod-freeze branch from aec671a to 697c17e Compare November 13, 2024 16:15
@joeyparrish joeyparrish changed the title fix(DASH): Fix playback of some DASH with multiple periods fix(DASH): Fix playback after DASH period eviction Nov 13, 2024
joeyparrish
joeyparrish approved these changes Nov 13, 2024
View reviewed changes
Copy link
Member

@joeyparrish joeyparrish left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems okay to me. @tykus160? @avelad?

@tykus160 tykus160 merged commit 5eff038 into shaka-project:main Nov 13, 2024
18 checks passed
This was referenced Nov 13, 2024
Closed
Closed
Closed
Closed
Open
avelad added a commit to avelad/shaka-player that referenced this pull request Nov 15, 2024
@avelad
Revert "fix(DASH): Fix playback after DASH period eviction (shaka-pro…
c33863a 
…ject#7519)"

This reverts commit 5eff038.
avelad pushed a commit that referenced this pull request Nov 15, 2024
@ncocaign @avelad
fix(DASH): Fix playback after DASH period eviction (#7519)
0f9ad7c 
With this change, closeSegmentIndex() of all streams of a removed period
are defered in StreamingEngine.onUpdate_()

Fixes #7516
@shaka-bot shaka-bot mentioned this pull request Nov 15, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tykus160 tykus160 tykus160 approved these changes

@avelad avelad avelad approved these changes

@joeyparrish joeyparrish joeyparrish approved these changes

@theodab theodab Awaiting requested review from theodab

Assignees
No one assigned
Labels
component: DASH The issue involves the MPEG DASH manifest format priority: P2 Smaller impact or easy workaround type: bug Something isn't working correctly
Projects
None yet
Milestone
v4.13
5 participants
@ncocaign @shaka-bot @joeyparrish @avelad @tykus160